Skip to content

backport: partial bitcoin#35984 - #7827

Merged
PastaPastaPasta merged 1 commit into
dashpay:developfrom
PastaPastaPasta:codex/security-psbt-single-guard
Oct 14, 2026
Merged

PastaPastaPasta merged 1 commit into
dashpay:developfrom
PastaPastaPasta:codex/security-psbt-single-guard

Conversation

@PastaPastaPasta

@PastaPastaPasta PastaPastaPasta commented Oct 7, 2026 •

Copy link
Copy Markdown
Member

Issue being fixed or feature implemented

When a signer explicitly requests SINGLE or SINGLE|ANYONECANPAY for a PSBT input that has no output at the same index, the legacy signature hash is the constant one, so the signature does not bind the transaction as the signer expects. SignTransaction() already skipped such inputs but SignPSBTInput() did not. Default ALL signing and the wallet's requested-sighash agreement checks limit exposure to explicit nondefault signing.

What was done?

Partial backport of bitcoin#35984 (only test hunks omitted; see below).

The missing-output check moves into MutableTransactionSignatureCreator::CreateSig, so PSBT and raw transaction signing share the same refusal, and the redundant raw-signing check is removed.

Dash adaptations:

  • Uses nHashType & 0x1f with the legacy SignatureHash, since Dash lacks SIGHASH_OUTPUT_MASK and the upstream signing-options interface.
  • The regression test is ported for P2PKH only (Dash has no segwit or taproot) and extended with SINGLE|ANYONECANPAY, ALL and raw-signing controls.
  • Omitted, hence partial: upstream's test_psbt_roundtrip/test_psbt_version hunks, which replace listunspent()[0] with picking a mature 50-coin coinbase. Both tests arrive with PSBTv2 (Implement BIP 370 PSBTv2 bitcoin/bitcoin#21283), which Dash does not have, so there is nothing to apply them to. No ordering-dependent listunspent()[0] call runs after the new test in Dash's rpc_psbt.py, so no Dash equivalent is needed.

How Has This Been Tested?

On macOS arm64 with the existing depends prefix and --enable-werror:

  • Full make -j1 passed before and after the change.
  • The new signing scenario failed on the baseline in both legacy and descriptor modes (input 1 finalized under SINGLE) and passes after the fix.
  • rpc_psbt.py passed in both descriptor and legacy variants, including the SINGLE|ANYONECANPAY, ALL and raw-signing controls.
  • src/test/test_dash --run_test=sighash_tests,script_tests,psbt_wallet_tests passed.
  • Targeted Python lint, whitespace lint and git diff --check passed.

Breaking Changes

The shared signer refuses to create SINGLE signatures when the matching output is missing. No consensus or RPC schema changes.

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

🤖 Generated with Claude Code

@thepastaclaw

thepastaclaw commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

🕓 Review not started yet because the new head is waiting for the 30-minute push debounce.

  • Request normal review — click when the PR is ready for review.
  • Request priority review — click to move this review to the front of the queue.

Commit ce5b786. Normal review starts when eligible; priority review starts as soon as a slot is available.

@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: eeea32fb-7dd4-482e-a313-ecf0e2b2c1ba
📥 Commits

Reviewing files that changed from the base of the PR and between b88580c and 59391d5.

📒 Files selected for processing (1)
  • test/functional/rpc_psbt.py

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

Walkthrough

CreateSig now rejects new SIGHASH_SINGLE signatures when the input has no corresponding output. SignTransaction attempts signature production for each input. A functional test checks PSBT and raw-transaction signing with SIGHASH_SINGLE, SIGHASH_SINGLE|ANYONECANPAY, and ALL.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 59391

This backport makes PSBT and raw transaction signing consistently refuse SIGHASH_SINGLE signatures when the input has no matching output. No merge-blocking risk was found.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description directly explains the missing-output signing issue, the shared signer fix, Dash-specific adaptations, omitted upstream hunks, and validation results.
Title check Passed The title clearly identifies this change as a partial backport of Bitcoin PR #35984, which matches the primary purpose of the changeset.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final validation — Phase 1 + Phase 2

Verified the complete two-file change at e5823ec against Bitcoin bitcoin#35984 and Dash's signing callers; no actionable defects were found. The omitted upstream UTXO-selection edits are explicitly excluded in the PR description and commit message, so they do not warrant a completeness finding. Validation was static only: the supplied CI snapshot shows successful container builds and formatting/title/merge checks, two queued manifest jobs, and no completed unit or functional test results.

Review provenance

Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: low by gpt-6.1-sol (effort low) — The diff moves a small missing-output guard into MutableTransactionSignatureCreator::CreateSig and adds focused regression coverage, making it contained and straightforward despite touching signing.
  • Phase 1 reviewers: glm-5.3-flash — general (completed, effort high); agent phase1-reviewer
  • Phase 1 model: glm-5.3-flash — zai quota: 5h 95% left, weekly 95% left; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 15% left, 5h 100% left)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — backport-reviewer (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort medium); agent phase2-reviewer

@thepastaclaw thepastaclaw added the pastaclaw:approved thepastaclaw's latest review approved this PR label Oct 7, 2026
@PastaPastaPasta
PastaPastaPasta force-pushed the codex/security-psbt-single-guard branch from e5823ec to 4e6d495 Compare October 8, 2026 23:49
@PastaPastaPasta PastaPastaPasta changed the title backport: guard SIGHASH_SINGLE signing (bitcoin#35984) backport: bitcoin#35984 Oct 8, 2026
@PastaPastaPasta PastaPastaPasta added this to the 24 milestone Oct 9, 2026

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

At head 4e6d495, the complete two-file diff correctly centralizes the missing-output SIGHASH_SINGLE refusal without changing consensus verification or ALL-based wallet and CoinJoin signing. The retained regression covers PSBT and raw signing, including SINGLE|ANYONECANPAY and ALL controls; the omitted upstream test-maintenance hunks are explicitly excluded in the PR description and do not warrant findings. Verification was static: the supplied CI snapshot reports successful completed builds, lint, and no-wallet tests, with wallet-enabled Linux tests, sanitizer tests, and the multiprocess build still pending.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)

Review provenance

Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: low by gpt-6.1-sol (effort low) — The change is a small, well-contained relocation of an existing SIGHASH_SINGLE guard into the shared signature creator, with focused regression coverage, rather than a large or intricate signature change.
  • Phase 1 reviewers: gemini-3.8-flash-high — general (completed, effort high); agent phase1-reviewer
  • Phase 1 model: gemini-3.8-flash-high — antigravity quota: weekly 74% left, 5h 36% left
  • Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — backport-reviewer (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — backport-reviewer (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort medium); agent phase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.

@PastaPastaPasta
PastaPastaPasta force-pushed the codex/security-psbt-single-guard branch 2 times, most recently from b88580c to 59391d5 Compare October 9, 2026 02:09

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

Verified the complete diff at 59391d5 against the upstream merge and surrounding signing paths; no actionable defects were found. The shared refusal matches Dash's legacy signature-hash semantics, and the omitted PSBTv2 test-fixture changes are explicitly documented intentional exclusions. This was a static review: the supplied CI snapshot shows successful lint and several platform builds, with three test jobs and additional builds still pending.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)

Review provenance

Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: backport-reviewer); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: low by gpt-6.1-sol (effort low) — The change moves a small, straightforward missing-output guard into the shared signature creator and adds focused regression tests, without the size or intricacy required for critical.
  • Phase 1 reviewers: gemini-3.8-flash-high — general (completed, effort high); agent phase1-reviewer
  • Phase 1 model: gemini-3.8-flash-high — antigravity quota: weekly 69% left, 5h 90% left
  • Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — backport-reviewer (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — backport-reviewer (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort medium); agent phase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.
Out-of-scope follow-up suggestions (1)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

  • Declared omission: PSBTv2-dependent upstream test transformations — INTENTIONAL_EXCLUSION: The supplied PR description explicitly states that upstream's test_psbt_roundtrip/test_psbt_version UTXO-selection hunks are omitted because their PSBTv2 tests are absent; HEAD's BACKPORT NOTE repeats this exclusion. Comparing upstream merge e19f83e with its first parent confirms that these two hunks replace listunspent()[0] with selection by amount == Decimal(50). The tests were introduced by 8838418 and bcc1dca, both verified ancestors of the bitcoin#21283 merge d7ed284. Neither test exists in Dash's base f35a24c or exact head, and base-history git log -S searches show no introduction. Their omission is therefore an expressly bounded backport exception, not an undisclosed prerequisite gap; requesting future PSBTv2 work is outside this fix's scope.
    • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.


raw = node.createrawtransaction(ins, [{wallet.getnewaddress(): 1.9999}])
for sighash in ["SINGLE", "SINGLE|ANYONECANPAY", "ALL"]:
signed = wallet.walletprocesspsbt(node.converttopsbt(raw), True, sighash)["psbt"]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes in test with .listunspent calls are not backported, I think better to mark bitcoin#35984 as partial

Otherwise it LGTM

… with no corresponding output

8df006f wallet: skip signing SIGHASH_SINGLE inputs with no corresponding output (furszy)

Pull request description:

  `SIGHASH_SINGLE` only commits to the output at the input's index. If the output at such
  position doesn't exist, it commits to no output at all (legacy uses a fixed sighash of 1, segwit
  v0 zeroes `hashOutputs`), which means the signature stays valid even when outputs are
  swapped, which is a footgun that lets funds be redirected without the owner's consent.

  `SignTransaction()` already skipped these inputs, but `SignPSBTInput()` did not, so
  `walletprocesspsbt` signed them. This moves the check into the `CreateSig` so both
  paths, and any future one, skip producing the detached signature.

  Note: if there is a valid use for the segwit v0 case, I would rather re-allow it through
  an explicit opt-in arg than by default, so it is always a deliberate choice.

  Fixes bitcoin#35977

ACKs for top commit:
  thomasbuilds:
    ACK 8df006f
  achow101:
    ACK 8df006f

Tree-SHA512: ac1c9659910eae889475c65b8dde68e711c4d85d0d520af06411728591e573bf027b41a9ac6dd0f6e93de425841ac6c7d9e6c9f07936b9d8ae390941fbe6f1f1

BACKPORT NOTE:
- src/script/sign.cpp: Dash has no SignOptions or SIGHASH_DEFAULT, so the
  check uses nHashType. Dash also has no SIGHASH_OUTPUT_MASK (added with
  Taproot), so it tests (nHashType & 0x1f), the same test Dash's legacy
  SignatureHash uses for SIGHASH_SINGLE. For every sighash the wallet can
  sign with (ALL/NONE/SINGLE, optionally |ANYONECANPAY) this matches
  upstream's (hashtype & SIGHASH_OUTPUT_MASK).
- test/functional/rpc_psbt.py, test_sighash_single():
  - No loop over legacy/bech32/bech32m address types; Dash only has P2PKH.
    Instead the test covers SINGLE, SINGLE|ANYONECANPAY and ALL (control),
    and also checks signrawtransactionwithwallet, because this change
    removes SignTransaction's own SIGHASH_SINGLE check and Dash had no test
    for that path.
  - It runs after test_input_confs_control; Dash has none of the sighash
    tests upstream places it next to.
  - Omitted: the test_psbt_roundtrip and test_psbt_version UTXO selection
    changes; those tests (PSBTv2) do not exist in Dash.

Co-authored-by: Ava Chow <github@achow101.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@PastaPastaPasta
PastaPastaPasta force-pushed the codex/security-psbt-single-guard branch from 59391d5 to ce5b786 Compare October 14, 2026 20:59
@PastaPastaPasta PastaPastaPasta changed the title backport: bitcoin#35984 backport: partial bitcoin#35984 Oct 14, 2026
@thepastaclaw thepastaclaw removed the pastaclaw:approved thepastaclaw's latest review approved this PR label Oct 14, 2026
@PastaPastaPasta
PastaPastaPasta merged commit 3dc3d05 into dashpay:develop Oct 14, 2026
22 of 23 checks passed
PastaPastaPasta added a commit that referenced this pull request Oct 17, 2026
ce5b786 partial Merge bitcoin#35984: sign: skip signing SIGHASH_SINGLE inputs with no corresponding output (pasta)

Pull request description:

  ## Issue being fixed or feature implemented

  When a signer explicitly requests `SINGLE` or `SINGLE|ANYONECANPAY` for a PSBT input that has no output at the same index, the legacy signature hash is the constant one, so the signature does not bind the transaction as the signer expects. `SignTransaction()` already skipped such inputs but `SignPSBTInput()` did not. Default `ALL` signing and the wallet's requested-sighash agreement checks limit exposure to explicit nondefault signing.

  ## What was done?

  Partial backport of bitcoin#35984 (only test hunks omitted; see below).

  The missing-output check moves into `MutableTransactionSignatureCreator::CreateSig`, so PSBT and raw transaction signing share the same refusal, and the redundant raw-signing check is removed.

  Dash adaptations:
  - Uses `nHashType & 0x1f` with the legacy `SignatureHash`, since Dash lacks `SIGHASH_OUTPUT_MASK` and the upstream signing-options interface.
  - The regression test is ported for P2PKH only (Dash has no segwit or taproot) and extended with `SINGLE|ANYONECANPAY`, `ALL` and raw-signing controls.
  - Omitted, hence partial: upstream's `test_psbt_roundtrip`/`test_psbt_version` hunks, which replace `listunspent()[0]` with picking a mature 50-coin coinbase. Both tests arrive with PSBTv2 (bitcoin#21283), which Dash does not have, so there is nothing to apply them to. No ordering-dependent `listunspent()[0]` call runs after the new test in Dash's `rpc_psbt.py`, so no Dash equivalent is needed.

  ## How Has This Been Tested?

  On macOS arm64 with the existing depends prefix and `--enable-werror`:
  - Full `make -j1` passed before and after the change.
  - The new signing scenario failed on the baseline in both legacy and descriptor modes (input 1 finalized under `SINGLE`) and passes after the fix.
  - `rpc_psbt.py` passed in both descriptor and legacy variants, including the `SINGLE|ANYONECANPAY`, `ALL` and raw-signing controls.
  - `src/test/test_dash --run_test=sighash_tests,script_tests,psbt_wallet_tests` passed.
  - Targeted Python lint, whitespace lint and `git diff --check` passed.

  ## Breaking Changes

  The shared signer refuses to create `SINGLE` signatures when the matching output is missing. No consensus or RPC schema changes.

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

  🤖 Generated with [Claude Code](https://claude.com/claude-code)

Top commit has no ACKs.

Tree-SHA512: 4848c67cdbb8f53429b1c3f2f7a80aea652426a198947b175137c3a8fe2381ab284cf8086950d4434f6eeafa5caa78822f2b38da8e51b2ae1fadcfe1e969fd61
(cherry picked from commit 3dc3d05)
PastaPastaPasta added a commit that referenced this pull request Oct 17, 2026
6c02b2b ci: let DEPENDS_ARCH override the machine arch for multiprocess and tsan (pasta)
ce5dbf3 Merge #7888: fix(consensus): canonical ExtNetInfo encoding and strict netInfo decoding (pasta)
54cba5c Merge #7537: fix: data races and check-then-act races in the CoinJoin server (pasta)
08d8bca Merge #7909: feat(mining): build the masternode list incrementally during block assembly (pasta)
5633761 Merge #7895: test: cover retroactive provider tx refusal, fix wording (pasta)
cbb9500 Merge #7846: fix: treat pending owner keys and Platform node IDs as one conflict namespace (pasta)
8efdf5e Merge #7857: fix: mine a pending ProUpServTx before a key change of its masternode (pasta)
e56e6ae Merge #7842: fix(wallet): reject mnemonic passphrases that seed derivation truncates (pasta)
79480ff test: follow-up for #7821: check if wallet is locked after failed upgrade instead creating brand-new scenario (Konstantin Akimov)
a56ed47 fix(net): announce sporks to new peers instead of requesting them during mnsync (pasta)
cdaea5f Merge #7894: fix: due to logical conflicts feature_pruning.py had been broken while (pasta)
98d92f6 Merge #7817: backport: partial bitcoin#34156 restore cleanup ownership (pasta)
35e6d18 Merge #7858: fix: do not InstantSend-lock provider transactions (pasta)
904d73d Merge #7881: fix: refuse to sign shared registrar updates that move the funder's coins (pasta)
95982f5 Merge #7782: fix(wallet): persist pending-observation coin locks once the record becomes readable (pasta)
cdf65d0 Merge #7836: backport: partial bitcoin#25812, bitcoin#29855 (pasta)
cc5716f Merge #7824: fix: retain initialized DKG sessions during inventory reads (pasta)
588e458 Merge #7827: backport: partial bitcoin#35984 (pasta)
795c6f1 Merge #7839: fix: release InstantSend tracking after mempool eviction (pasta)
dc1a6b2 Merge #7875: docs(rpc): clarify getchainlockbyheight availability (pasta)
e4798ff Merge #7876: fix(consensus): capture operator key scheme changes with unchanged bytes in state diffs (pasta)
fe68ecb Merge #7871: backport!: Merge bitcoin#27217: wallet: Replace use of purpose strings with an enum (pasta)
8348d22 Merge #7867: fix: refuse masternode list RPCs for blocks outside the active chain (pasta)
861a579 Merge #7866: fix: stop `masternode payments` at the genesis block (pasta)
efcef71 Merge #7521: fix: update masternode rate limit on failed governance trigger path (pasta)
0623182 Merge #7831: fix: bound governance object signatures and reset the governance cache (pasta)
b230657 Merge #7841: fix: authenticate cached InstantSend proof bytes (pasta)
a1f37ea Merge #7820: fix: hold chainstate lock during CoinJoin input validation (pasta)
50a5617 Merge #7829: fix: preserve wallet permissions when changing passphrases (pasta)
2ff0103 Merge #7818: fix: return REST errors for unavailable deployment data (pasta)
cb39369 Merge #7834: fix: validate Electrum import input boundaries (pasta)
e7e3daa Merge #7821: fix: relock encrypted wallets after HD upgrade failures (pasta)
2fea45f Merge #7792: fix: treat missing snapshots during evodb repair as repair errors (pasta)
3c5c039 Merge #7838: fix(wallet): authenticate unlock deadline extensions (pasta)
0bf80f8 Merge #7810: fix(rpc): handle unavailable masternode payment inputs (pasta)
bcefee4 Merge #7822: fix: reject malformed detached quorum share batches (pasta)
86a7737 Merge #7828: fix: omit recovery words from invalid mnemonic errors (pasta)
886d5c4 Merge #7816: fix: check credit pool amount accumulation (pasta)
00fc3b9 Merge #7851: fix(rpc): correct masternode status help for top-level service field (pasta)
0e75c22 Merge #7814: fix(qt): restore HTML escaping of send confirmation recipients (pasta)

Pull request description:

  ## Issue being fixed or feature implemented

  Backports for v24.0.0-rc.4, on top of `v24.0.x` at the v24.0.0-rc.3 tag (0364ed6, #7808).

  ## What was done?

  Cherry-picked (`git cherry-pick -m1 -x`) every develop merge since rc.3 that carries `backport-candidate-24.0.x`, plus eight unlabeled fixes picked after an audit of every other develop merge since rc.3 (marked † below). They are in develop merge order:

  - #7814 fix(qt): restore HTML escaping of send confirmation recipients †
  - #7851 fix(rpc): correct masternode status help for top-level service field
  - #7816 fix: check credit pool amount accumulation
  - #7828 fix: omit recovery words from invalid mnemonic errors †
  - #7822 fix: reject malformed detached quorum share batches †
  - #7810 fix(rpc): handle unavailable masternode payment inputs †
  - #7838 fix(wallet): authenticate unlock deadline extensions
  - #7792 fix: treat missing snapshots during evodb repair as repair errors
  - #7821 fix: relock encrypted wallets after HD upgrade failures
  - #7834 fix: validate Electrum import input boundaries
  - #7818 fix: return REST errors for unavailable deployment data
  - #7829 fix: preserve wallet permissions when changing passphrases
  - #7820 fix: hold chainstate lock during CoinJoin input validation
  - #7841 fix: authenticate cached InstantSend proof bytes
  - #7831 fix: bound governance object signatures and reset the governance cache
  - #7521 fix: update masternode rate limit on failed governance trigger path
  - #7866 fix: stop `masternode payments` at the genesis block
  - #7867 fix: refuse masternode list RPCs for blocks outside the active chain †
  - #7871 backport!: bitcoin#27217: wallet: Replace use of purpose strings with an enum
  - #7876 fix(consensus): capture operator key scheme changes with unchanged bytes in state diffs
  - #7875 docs(rpc): clarify getchainlockbyheight availability †
  - #7839 fix: release InstantSend tracking after mempool eviction
  - #7827 backport: partial bitcoin#35984
  - #7824 fix: retain initialized DKG sessions during inventory reads
  - #7836 backport: partial bitcoin#25812, bitcoin#29855
  - #7782 fix(wallet): persist pending-observation coin locks once the record becomes readable
  - #7881 fix: refuse to sign shared registrar updates that move the funder's coins †
  - #7858 fix: do not InstantSend-lock provider transactions
  - #7817 backport: partial bitcoin#34156 restore cleanup ownership
  - #7894 test: fix feature_pruning.py restart arguments † (needed because #7792 makes the evodb repair fatal, which the old restart arguments trip)
  - #7893 fix(net): announce sporks to new peers instead of requesting them during mnsync
  - 4a19742 (from #7859) test: follow-up for #7821: check if wallet is locked after failed upgrade instead creating brand-new scenario. Test-only, picked as its commit. Without it, #7821's `test_failed_upgrade()` expects RPC error -1 for a 257-byte mnemonic passphrase, which #7842's new precheck rejects with -8 first, so `wallet_upgradetohd.py --legacy-wallet` fails. On develop, #7859 replaced that scenario before #7842 merged. The rest of #7859 is test cleanup and docs and is left out.
  - #7842 fix(wallet): reject mnemonic passphrases that seed derivation truncates
  - #7857 fix: mine a pending ProUpServTx before a key change of its masternode
  - #7846 fix: treat pending owner keys and Platform node IDs as one conflict namespace
  - #7895 test: cover retroactive provider tx refusal, fix wording
  - #7909 feat(mining): build the masternode list incrementally during block assembly
  - #7537 fix: data races and check-then-act races in the CoinJoin server
  - #7888 fix(consensus): canonical ExtNetInfo encoding and strict netInfo decoding

  34 of the 39 picks change exactly the same lines as their develop merges. #7888 and #7782 needed test-only changes (see testing), and three others needed adaptation, described in their commit messages:

  - **#7893** was merged to develop twice: f99212b with an earlier revision of its commit, then 72a5997 with the final one, so develop now announces sporks twice in the VERACK handler (a develop fix is open separately). Here the PR's final commit 45acdf3 is picked once instead of either merge. In `src/node/sync_manager.cpp`, this branch still sent `GETSPORKS` from the mnsync tick on every network, because #7847, which limited it to test chains, is not included. The request is removed entirely, as on develop. Mainnet spork values are hardcoded in `CSporkManager::GetSporkValue`, so the mainnet request had no effect.
  - **#7857** and **#7909**: develop folded `CChainstateHelper` into `Chainstate` (d5c1bd7), which is not on this branch, so the new code goes through `ChainHelper().special_tx` / `m_chain_helper.special_tx` instead of `m_special_tx`. The neighbouring develop-only `CSpecialTxProcessor::GetDeterministicMNListHash` is left out.

  #7888 tightens consensus rules for ExtAddr provider transactions behind V24, so I checked it against testnet. V24 is not active there yet (the tip's CbTx is still version 3), so the chain has no ExtAddr ProTxs the new rules could reject. rc.3's own serializer always writes canonical, sorted purposes, so every ProTx it builds passes the new rules. Mixed rc.3/rc.4 networks could only split on a hand-crafted non-canonical ExtAddr ProTx mined after V24 activates. So testnet miners and masternodes should move to rc.4 before V24 is signalled.

  Unlabeled develop fixes since rc.3 that were audited and left out:
  - Worth considering before the final release: #7869 (startup block replay after an unclean shutdown asserts on a quorum commitment, so recovery needs `-reindex-chainstate`; not a v24 regression), #7833 (gate signing intake before quorum retrieval, DoS hardening), #7682 (intermittent `relic_conf.h` parallel-build failure).
  - Left for 24.1: #7812, #7826, #7844, #7847, #7850, #7868, plus the routine upstream backports (#7780, #7806, #7830, #7855, #7856, #7862, #7863, #7865, #7883, #7884, #7898, #7906), refactors (#7805, #7878, #7880), and CI, docs and test-only changes.

  One CI-only commit comes last. It takes only the `ci/test` hunks of 6f71fa5 (#7901), which make `ci/test/00_setup_env_native_{multiprocess,tsan}.sh` honor `DEPENDS_ARCH`. Pull requests into `v24.0.x` run develop's workflow files (`pull_request_target`), and since #7901 those check the depends cache on x86_64 runners and pass the aarch64 build arch in `DEPENDS_ARCH`. Without the hunks, `linux64_multiprocess / Build depends` fails on any depends cache miss (`Build runner computes HOST=aarch64-linux-gnu but check-cache used x86_64-pc-linux-gnu`). Without `DEPENDS_ARCH` the scripts still use `uname -m`, so this branch's own push CI is unchanged.

  No version change is needed. `configure.ac` is already 24.0.0 with `CLIENT_VERSION_IS_RELEASE` false, so the build takes its version string from the `v24.0.0-rc.4` tag.

  ## How Has This Been Tested?

  Built and tested at 6c02b2b on macOS arm64 against depends, with `--enable-debug --enable-werror --disable-hardening`.

  - `make check`: all unit test suites pass, including `test_dash-qt`.
  - Functional tests, all passing: `feature_evo_shares_activation.py`, `feature_governance_objects.py`, `feature_llmq_signing.py` (plain and `--spork21`), `feature_masternode_shares.py`, `feature_sporks.py`, `interface_rest.py`, `p2p_instantsend.py`, `p2p_quorum_data.py`, `rpc_masternode.py`, `rpc_psbt.py`, `rpc_quorum.py`, `wallet_backup.py`, `wallet_dump.py`, `wallet_encryption.py`, `wallet_labels.py` and `wallet_upgradetohd.py`, each with `--descriptors` and `--legacy-wallet` where it supports them.

  - Each pick's changed lines were compared with its develop merge (or, for #7893, its PR commit). They are identical except for the #7893, #7857 and #7909 adaptations above, and the #7888 test adaptation below.
  - A local macOS arm64 debug build (`--enable-debug --enable-werror`) compiled most of the tree and stopped on a single error in `src/test/evo_netinfo_tests.cpp`. #7888's new test used `CNetAddr::V2()`, the serialization-parameter API from bitcoin#25284, which this branch lacks. The pick now writes that entry through an `OverrideStream` with `ADDRV2_FORMAT`, the way `NetInfoEntry::Serialize` does on this branch (see its commit message).
  - #7782's new unit test reloads the wallet with develop's three-argument `CWallet` constructor. On this branch the constructor still takes the CoinJoin loader and an `ArgsManager`, so the test passes them the way its fixture already does (see the commit message).
  - With both test fixes, a full local `make` (`dashd`, `dash-qt`, `test_dash`, `test_dash-qt`) completes with no errors.
  - `feature_pruning.py` is an extended test that CI does not run. #7894 is included because #7792 makes its restart path fail.

  ## Breaking Changes

  Carried over from the picked PRs:

  - #7876: rc.1 to rc.3 nodes that connected an affected block stored a wrong masternode list diff for it. Those nodes need `-reindex`; `-reindex-chainstate` is not enough for nodes that already marked the following block invalid.
  - #7888: stricter ExtAddr `netInfo` decoding and validation behind V24 (see above). The RFC1918 check now applies even with `-allowprivatenet`.
  - #7858: provider transactions no longer get InstantSend locks. Software that waits for an islock on a ProTx should wait for a ChainLock or confirmations instead.
  - #7857: a `ProUpServTx` spending an output of a pending operator key change, asset lock, asset unlock or MNHF signal is rejected from the mempool until that transaction confirms.
  - #7831: governance object signatures over 96 bytes are rejected, and the governance cache is discarded and re-synced on first start (`doc/release-notes-7831.md`).
  - #7871: address purposes are limited to `send`, `receive` and `refund`. Wallets with other purpose strings load with warnings, and `listlabels` errors on an unknown purpose (`doc/release-notes-27217.md`).
  - #7842: creating an HD wallet with a mnemonic passphrase over 248 bytes fails unless `-allow-deprecated-long-mnemonics` is set, and over 256 bytes always fails (`doc/release-notes-7842.md`).
  - #7792: a node whose evodb is missing a masternode list snapshot now stops at startup and asks for `-reindex`, and `evodb repair` reports it in `repairErrors`.
  - #7867: `protx info <proTxHash> <blockHash>`, `protx listdiff` and `masternode payments <blockHash>` return an error for blocks outside the active chain.
  - #7881: `protx shared_combine` on a `ProUpSharedRegTx` is refused unless its inputs are unlocked coins of a single address of the calling wallet, change returns to that address, and the fee is within `-maxtxfee`.
  - #7836: PSBTs referencing nonexistent previous outputs fail to deserialize (RPC error -22).
  - #7834: negative Electrum-import rescan indices are rejected.
  - #7893: on this branch, mainnet nodes no longer send `GETSPORKS` during mnsync (no effect, mainnet spork values are hardcoded).

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone

  🤖 Generated with [Claude Code](https://claude.com/claude-code)

Top commit has no ACKs.

Tree-SHA512: 7de9535ca51620f4ddf5a46df2c3341a5e5fdd2263606f83f7db6b576c31ff7b82e1b285bf66988ac4b2729a9060513c45710135f61d7fd5c147b670d5f405dc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants